New pose proposal: support rotation in degrees (#252) #589

aaronchongth · 2021-06-11T10:33:07Z

🎉 New feature

Related to #252. This has been considered as more favorable than #581 (closed).

Summary

Param values can now be parsed differently according to their parent Element's attributes. This is being used by Pose in this PR.
The rotation units can now be parsed as degrees with the attribute //pose[@degrees='true'], otherwise it is parsed as radians by default.
Added tests to handle additional spaces or newlines as delimiters between translation and rotation values.

Valid poses look like so,

<!-- Pose with rotation units in radians -->
<pose>1 2 3   0.4 0.5 0.6</pose>

<!-- Pose with rotation units in radians -->
<pose degrees="false">1 2 3   0.4 0.5 0.6</pose>

<!-- Pose with rotation units in degrees -->
<pose degrees="true">1 2 3   0.4 0.5 0.6</pose>

<!-- Single space delimiter -->
<pose>1 2 3 0.4 0.5 0.6</pose>

<!-- Multiple space delimiters -->
<pose>1 2 3   0.4 0.5 0.6</pose>

<!-- Newline delimiters -->
<pose>
  1 2 3
  0.4 0.5 0.6
</pose>

Implementation details,

Added API for SetParentElement and GetParentElement, modified Element::Clone, Element::Copy to handle setting parent elements for each child value or attribute.
Added Reparse function to Param, to be called when parent Element attributes have been changed manually, and changes to its value need to be reflected.
SetParentElement attempts to reparse previous string-ified values using new parent element attributes.
Added ignoreParentAttribute flag, for when the value of the Param was set explicitly using the Set<T> function. The desired behavior would be that explicitly set values should not be affected by Reparse, while values SetFromString, should take into account of parent Element attributes.

Test it

source install/setup.bash
colcon test --merge-install --packages-select sdformat12
colcon test-result

Checklist

Signed all commits for DCO
Added tests
Added example and/or tutorial
Updated documentation (as needed)
Updated migration guide (as needed)
Code check passed (In source directory, run sh tools/code_check.sh)
All tests passed (See test coverage)
While waiting for a review on your PR, please help review
another open pull request
to support the maintainers

Note to maintainers: Remember to use Squash-Merge

azeey · 2021-06-25T16:20:47Z

@aaronchongth My understanding according the the updated proposal was that the "simpler" implementation would use the //pose/@rotation_type attribute instead of //pose/translation and //pose/rotation. So for the examples you gave, it would be:

<!-- Read as (x y z roll pitch yaw), with rotation in radians -->
<pose>1 2 3   0.4 0.5 0.6</pose>

<!-- Read as (x y z roll pitch yaw), with rotation in radians -->
<pose rotation_type="rpy_radians">1 2 3   0.4 0.5 0.6</pose>

<!-- Read as (x y z roll pitch yaw), with rotation in degrees -->
<pose rotation_type="rpy_degrees">1 2 3   0.4 0.5 0.6</pose>

<!-- Read as (x y z W X Y Z), with rotation in quaternions -->
<pose rotation_type="rpy_qwxyz">1 2 3   0.707 0.707 0 0</pose>

Do you mind updating this PR to support //pose/@rotation_type?

aaronchongth · 2021-06-28T08:40:35Z

Thanks for pointing it out, @azeey! I have just made the changes, and tried to adhere more to the proposal, 8480dd6

codecov-commenter · 2021-06-28T09:04:08Z

Codecov Report

Merging #589 (36d9f6b) into main (6a6aba1) will decrease coverage by 0.07%.
The diff coverage is 71.11%.

❗ Current head 36d9f6b differs from pull request most recent head 8480dd6. Consider uploading reports for the commit 8480dd6 to get more accurate results

@@            Coverage Diff             @@
##             main     #589      +/-   ##
==========================================
- Coverage   87.54%   87.46%   -0.08%     
==========================================
  Files          72       72              
  Lines       10514    10557      +43     
==========================================
+ Hits         9204     9234      +30     
- Misses       1310     1323      +13

Impacted Files	Coverage Δ
src/Param.cc	`89.56% <65.78%> (-2.80%)`	⬇️
src/Utils.cc	`86.56% <100.00%> (+0.62%)`	⬆️

Continue to review full report at Codecov.

Legend - Click here to learn more
Δ = absolute <relative> (impact), ø = not affected, ? = missing data
Powered by Codecov. Last update 6a6aba1...8480dd6. Read the comment docs.

EricCousineau-TRI · 2021-06-28T13:57:17Z

<!-- Read as (x y z   W X Y Z), with rotation in quaternions, this is an interesting case, more explanations below -->
<pose>1 2 3   0.707 0.707 0 0</pose>

This has a weird smell; I think this code (if it works) is doing too much. If @rotation_type isn't specified, then the semantics should only be that of rpy_radians; nothing additional should be inferred from malformed / different sized values, IMO.
Otherwise, an innocent typo can be accepted in a very confusing way.

EricCousineau-TRI

First pass done, PTAL!

src/Param.cc

src/Utils.cc

codecov-commenter · 2021-07-08T10:02:03Z

Codecov Report

Merging #589 (95c292e) into main (c919d23) will decrease coverage by 0.08%.
The diff coverage is 80.58%.

@@            Coverage Diff             @@
##             main     #589      +/-   ##
==========================================
- Coverage   87.98%   87.90%   -0.09%     
==========================================
  Files          72       72              
  Lines       10929    11022      +93     
==========================================
+ Hits         9616     9689      +73     
- Misses       1313     1333      +20

Impacted Files	Coverage Δ
include/sdf/Element.hh	`100.00% <ø> (ø)`
src/Param.cc	`89.18% <77.01%> (-3.31%)`	⬇️
include/sdf/Param.hh	`77.14% <100.00%> (ø)`
src/Element.cc	`96.82% <100.00%> (+0.06%)`	⬆️
src/parser.cc	`84.93% <100.00%> (ø)`

Continue to review full report at Codecov.

Legend - Click here to learn more
Δ = absolute <relative> (impact), ø = not affected, ? = missing data
Powered by Codecov. Last update c919d23...95c292e. Read the comment docs.

azeey

Looks pretty good. I have a few minor comments. Can you also fix the merge conflicts when you get a chance?

include/sdf/Element.hh

sdf/1.9/actor.sdf

sdf/1.9/inertial.sdf

sdf/1.9/urdf.sdf

src/Element.cc

src/Param.cc

test/integration/param_parent_element.cc

Signed-off-by: Aaron Chong <[email protected]>

… sdf Signed-off-by: Aaron Chong <[email protected]>

Signed-off-by: Aaron Chong <[email protected]>

…function Signed-off-by: Aaron Chong <[email protected]>

Signed-off-by: Aaron Chong <[email protected]>

azeey · 2021-08-30T14:54:20Z

Per VC, Aaron to fix printing of poses so that if degrees=true, poses will be printed in degrees.

Signed-off-by: Aaron Chong <[email protected]>

aaronchongth · 2021-08-31T17:21:01Z

Thanks for the note, @azeey! This commit 9ac3504 should be fixing the problem and contains some tests too. The expected values from certain models in other tests have changed as well, I have made the necessary changes.

azeey

Looking great! Just a few more minor comments.

include/sdf/Param.hh

src/Param.cc

include/sdf/Param.hh

Signed-off-by: Aaron Chong <[email protected]>

…eparse Signed-off-by: Aaron Chong <[email protected]>

Signed-off-by: Aaron Chong <[email protected]>

src/Param.cc

Signed-off-by: Aaron Chong <[email protected]>

azeey

Great work!

Reparse was added in #589 to deal with the possibility that the meaning of an Element's value depends on the attributes in the element and those attributes might be different when reparenting. However, this does not apply when cloning since there will be no change in the attributes in the cloned element. --------- Signed-off-by: Arjo Chakravarty <[email protected]> Signed-off-by: Arjo Chakravarty <[email protected]> Signed-off-by: Addisu Z. Taddese <[email protected]> Co-authored-by: Steve Peters <[email protected]> Co-authored-by: Addisu Z. Taddese <[email protected]>

github-actions bot added the 🏯 fortress Ignition Fortress label Jun 11, 2021

aaronchongth requested a review from azeey June 11, 2021 10:35

aaronchongth force-pushed the aaron/pose-configurable-rotation branch from bd76451 to 01b01d6 Compare June 11, 2021 10:57

aaronchongth requested a review from EricCousineau-TRI June 14, 2021 14:08

aaronchongth force-pushed the aaron/pose-configurable-rotation branch from 95da234 to 8480dd6 Compare June 28, 2021 08:43

EricCousineau-TRI suggested changes Jun 28, 2021

View reviewed changes

src/Param.cc Outdated Show resolved Hide resolved

src/Utils.cc Outdated Show resolved Hide resolved

src/Utils.cc Outdated Show resolved Hide resolved

aaronchongth requested a review from EricCousineau-TRI July 9, 2021 10:01

azeey requested changes Jul 20, 2021

View reviewed changes

aaronchongth changed the title ~~New pose proposal - minimal (#252)~~ New pose proposal (#252) Jul 26, 2021

aaronchongth force-pushed the aaron/pose-configurable-rotation branch from bdfbcb9 to d4c8dcf Compare July 26, 2021 09:32

aaronchongth added 8 commits July 26, 2021 17:48

Added rotation type attribute to pose, handled parsing in param

7d251f9

Signed-off-by: Aaron Chong <[email protected]>

Using degrees boolean attribute in pose instead, added tests and test…

6874cf7

… sdf Signed-off-by: Aaron Chong <[email protected]>

Fix code check

582f80f

Signed-off-by: Aaron Chong <[email protected]>

Changed attribute to @rotation_type, modified tests, added delimiters

863b5c7

Signed-off-by: Aaron Chong <[email protected]>

Passing attributes into param value parsing, reverted Utils loadPose …

81f7087

…function Signed-off-by: Aaron Chong <[email protected]>

Reverted location of parsing nested sdf

347ae2e

Signed-off-by: Aaron Chong <[email protected]>

Added parent element for param, more tests

ec4a1a9

Signed-off-by: Aaron Chong <[email protected]>

Const ref for GetAttributes and it's relevant calls

f3e091e

Signed-off-by: Aaron Chong <[email protected]>

aaronchongth force-pushed the aaron/pose-configurable-rotation branch from c69920f to 40f2575 Compare August 31, 2021 06:24

Support printing pose elements in degrees, added tests

9ac3504

Signed-off-by: Aaron Chong <[email protected]>

aaronchongth force-pushed the aaron/pose-configurable-rotation branch from 40f2575 to 9ac3504 Compare August 31, 2021 16:32

azeey reviewed Sep 1, 2021

View reviewed changes

include/sdf/Param.hh Outdated Show resolved Hide resolved

src/Param.cc Outdated Show resolved Hide resolved

src/Param.cc Outdated Show resolved Hide resolved

include/sdf/Param.hh Outdated Show resolved Hide resolved

aaronchongth added 2 commits September 2, 2021 10:52

Removed const for SetParentElement

2099a3c

Signed-off-by: Aaron Chong <[email protected]>

Adding additional documentation for the behavior between Set<T> and R…

8602180

…eparse Signed-off-by: Aaron Chong <[email protected]>

aaronchongth force-pushed the aaron/pose-configurable-rotation branch from e901780 to 8602180 Compare September 2, 2021 06:18

aaronchongth added 3 commits September 2, 2021 15:48

Storing parsed tokens in array instead of vector

b0daab5

Signed-off-by: Aaron Chong <[email protected]>

Used explicit Get<boo> for degrees attribute

dabf47a

Signed-off-by: Aaron Chong <[email protected]>

Fixed missing array include for windows build

181e925

Signed-off-by: Aaron Chong <[email protected]>

aaronchongth requested a review from azeey September 2, 2021 08:14

azeey reviewed Sep 2, 2021

View reviewed changes

src/Param.cc Outdated Show resolved Hide resolved

Check for valueIndex, instead of array size

95c292e

Signed-off-by: Aaron Chong <[email protected]>

aaronchongth requested a review from azeey September 2, 2021 17:06

azeey approved these changes Sep 2, 2021

View reviewed changes

aaronchongth merged commit 96ac74e into main Sep 2, 2021

aaronchongth deleted the aaron/pose-configurable-rotation branch September 2, 2021 17:25

aaronchongth mentioned this pull request Sep 7, 2021

Support quaternions representation for poses #690

Merged

8 tasks

azeey mentioned this pull request Sep 9, 2021

Update document to better reflect proposed and accepted implementations gazebosim/sdf_tutorials#56

Merged

scpeters mentioned this pull request Sep 10, 2021

Fix/include pose degrees #697

Merged

7 tasks

scpeters mentioned this pull request Sep 9, 2024

[Do Not merge] Attempting to see if adding a cache will speed up the sdf::Root construction #1479

Draft

8 tasks

azeey mentioned this pull request Dec 20, 2024

Don't reparse parent elements when cloning. #1484

Merged

8 tasks

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

New pose proposal: support rotation in degrees (#252) #589

New pose proposal: support rotation in degrees (#252) #589

aaronchongth commented Jun 11, 2021 •

edited

Loading

azeey commented Jun 25, 2021

aaronchongth commented Jun 28, 2021 •

edited

Loading

codecov-commenter commented Jun 28, 2021 •

edited

Loading

EricCousineau-TRI commented Jun 28, 2021 •

edited

Loading

EricCousineau-TRI left a comment

codecov-commenter commented Jul 8, 2021 •

edited

Loading

azeey left a comment

azeey commented Aug 30, 2021

aaronchongth commented Aug 31, 2021

azeey left a comment

azeey left a comment

New pose proposal: support rotation in degrees (#252) #589

New pose proposal: support rotation in degrees (#252) #589

Conversation

aaronchongth commented Jun 11, 2021 • edited Loading

🎉 New feature

Summary

Test it

Checklist

azeey commented Jun 25, 2021

aaronchongth commented Jun 28, 2021 • edited Loading

codecov-commenter commented Jun 28, 2021 • edited Loading

Codecov Report

EricCousineau-TRI commented Jun 28, 2021 • edited Loading

EricCousineau-TRI left a comment

Choose a reason for hiding this comment

codecov-commenter commented Jul 8, 2021 • edited Loading

Codecov Report

azeey left a comment

Choose a reason for hiding this comment

azeey commented Aug 30, 2021

aaronchongth commented Aug 31, 2021

azeey left a comment

Choose a reason for hiding this comment

azeey left a comment

Choose a reason for hiding this comment

aaronchongth commented Jun 11, 2021 •

edited

Loading

aaronchongth commented Jun 28, 2021 •

edited

Loading

codecov-commenter commented Jun 28, 2021 •

edited

Loading

EricCousineau-TRI commented Jun 28, 2021 •

edited

Loading

codecov-commenter commented Jul 8, 2021 •

edited

Loading